Api: return users details by groups #8904

skjnldsv · 2018-03-20T18:08:21Z

This allows to get the users detailed data directly by requesting the groupid

$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details').then((response)=>console.log(response))
$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=10').then((response)=>console.log(response))
$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=2&offset=2').then((response)=>console.log(response))

nickvergessen · 2018-03-22T09:50:15Z

apps/provisioning_api/lib/Controller/UsersController.php

+		// Check subadmin has access to this group
+		if($this->groupManager->isAdmin($user->getUID())
+		   || $isSubadminOfGroup) {
+			$users = $this->groupManager->get($groupId)->getUsers();


you already got the group in 210,
also here the null check is missing.

nickvergessen · 2018-03-22T09:51:13Z

apps/provisioning_api/lib/Controller/UsersController.php

+		$user = $this->userSession->getUser();
+
+		// Check the group exists
+		if(!$this->groupManager->groupExists($groupId)) {


I suggest to get the group here and use the check for null to error out.
If you got a group don't check it any further

codecov · 2018-03-22T14:40:47Z

Codecov Report

Merging #8904 into master will decrease coverage by 0.09%.
The diff coverage is 58.4%.

@@             Coverage Diff             @@
##             master    #8904     +/-   ##
===========================================
- Coverage     51.97%   51.87%   -0.1%     
+ Complexity    25287    25285      -2     
===========================================
  Files          1601     1602      +1     
  Lines         94984    94996     +12     
  Branches       1388     1388             
===========================================
- Hits          49366    49283     -83     
- Misses        45618    45713     +95

Impacted Files	Coverage Δ	Complexity Δ
...isioning_api/composer/composer/autoload_static.php	`0% <ø> (ø)`	`1 <0> (ø)`	⬇️
apps/provisioning_api/appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ioning_api/composer/composer/autoload_classmap.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...rovisioning_api/lib/Controller/UsersController.php	`75% <100%> (+1.63%)`	`127 <0> (-13)`	⬇️
...ovisioning_api/lib/Controller/GroupsController.php	`68.88% <38.63%> (-23.97%)`	`27 <14> (+2)`
apps/provisioning_api/lib/Controller/AUserData.php	`68.11% <68.11%> (ø)`	`12 <12> (?)`
lib/private/Files/ObjectStore/Swift.php	`0% <0%> (-72.42%)`	`9% <0%> (ø)`
lib/private/Files/ObjectStore/SwiftFactory.php	`0% <0%> (-51%)`	`37% <0%> (ø)`
apps/files/lib/Controller/ViewController.php	`77.19% <0%> (-1.15%)`	`18% <0%> (ø)`
lib/private/Files/Mount/Manager.php	`100% <0%> (ø)`	`23% <0%> (-2%)`	⬇️
... and 6 more

skjnldsv · 2018-03-22T17:23:29Z

Ready for review again.
@nickvergessen

juliusknorr · 2018-03-22T20:12:58Z

apps/provisioning_api/appinfo/routes.php

@@ -42,6 +42,7 @@
 		// Users
 		['root' => '/cloud', 'name' => 'Users#getUsers', 'url' => '/users', 'verb' => 'GET'],
 		['root' => '/cloud', 'name' => 'Users#getUsersDetails', 'url' => '/users/details', 'verb' => 'GET'],
+		['root' => '/cloud', 'name' => 'Users#getUsersGroupDetails', 'url' => '/users/{groupId}/details', 'verb' => 'GET'],


The route doesn't really fit the schema of the others. Maybe something like /groups/{groupId}/users would make more sense.

I asked myself the same question, and was rather confuse, do we extract tehe users by groups or do we get the users in group :)
Therefore my current choice, especially since the getUsersData function is in the UsersController :)

And since /groups/{groupId}/ actually return the users, shouldn't we go for something like /groups/{groupId}/details? It really lokks like groups-informations related data. I would prefer something like /groups/{groupId}/users/details but /groups/{groupId}/users doesn't exists :/

/groups/{groupId}/details sounds good to me.

Such url is supposed to return the details of the group, like we have in /groups/details :)

I had teh same weird feeling on the url,
it doesn't feel right to me, maybe a ?group=<filter> on the current endpoint is the better option?

juliusknorr · 2018-03-22T20:21:12Z

apps/provisioning_api/lib/Controller/UsersController.php

+		$isSubadminOfGroup = false;
+		$group = $this->groupManager->get($groupId);
+		if ($group !== null) {
+			$isSubadminOfGroup = $this->groupManager->getSubAdmin()->isSubAdminofGroup($user, $group);


isSubAdminofGroup -> isSubAdminOfGroup

juliusknorr · 2018-03-22T20:31:53Z

apps/provisioning_api/lib/Controller/UsersController.php

+		if ($group !== null) {
+			$isSubadminOfGroup = $this->groupManager->getSubAdmin()->isSubAdminofGroup($user, $group);
+		} else {
+			throw new OCSException('The requested group could not be found', \OCP\API::RESPOND_NOT_FOUND);


\OCP\API is deprecated, see OCSNotFoundException

juliusknorr · 2018-03-22T20:32:48Z

apps/provisioning_api/lib/Controller/UsersController.php

+			}, $users);
+			$users = array_slice(array_values($users), $offset, $limit);
+		} else {
+			throw new OCSException('User does not have access to specified group', \OCP\API::RESPOND_UNAUTHORISED);


\OCP\API is deprecated, see OCSForbiddenException

skjnldsv · 2018-03-23T17:24:33Z

Ready for reviews again! :)

$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details').then((response)=>console.log(response))
$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=10').then((response)=>console.log(response))
$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=2&offset=2').then((response)=>console.log(response))

Fixed

skjnldsv · 2018-03-23T17:26:10Z

apps/provisioning_api/appinfo/routes.php

@@ -36,6 +36,8 @@
 		['root' => '/cloud', 'name' => 'Groups#getGroups', 'url' => '/groups', 'verb' => 'GET'],
 		['root' => '/cloud', 'name' => 'Groups#getGroupsDetails', 'url' => '/groups/details', 'verb' => 'GET'],
 		['root' => '/cloud', 'name' => 'Groups#getGroup', 'url' => '/groups/{groupId}', 'verb' => 'GET'],
+		['root' => '/cloud', 'name' => 'Groups#getGroupUsers', 'url' => '/groups/{groupId}/users', 'verb' => 'GET'],


Fallback to proper url design, above one is deprecated!

Could you add a note about this to #7827? But I guess it's not easily possible to drop this as it is part of the OCS standard 😉

nickvergessen · 2018-03-26T11:21:55Z

apps/provisioning_api/lib/Controller/GroupsController.php

 	 */
 	public function __construct(
 			string $appName,
 			IRequest $request,
 			IGroupManager $groupManager,
 			IUserSession $userSession,
-			ILogger $logger) {
+			ILogger $logger,
+			UsersController $userController) {


Instead extract the shared logic in a subclass/trait?

@nickvergessen what would you suggest to have a clean logic?

nickvergessen · 2018-03-26T11:22:08Z

apps/provisioning_api/lib/Controller/GroupsController.php

 use OCP\AppFramework\OCSController;
 use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\ILogger;
 use OCP\IRequest;
 use OCP\IUserSession;
 use OCP\IUser;
+use OCA\Provisioning_API\Controller\UsersController;


this should not be needed

nickvergessen · 2018-03-26T11:22:43Z

apps/provisioning_api/lib/Controller/GroupsController.php

-			$offset = (int)$offset;
-		}
-
+	public function getGroupsDetails(string $search = '', int $limit = null, int $offset = 0): DataResponse {


limit is not checked for null anymore?

Since we use slice to cut our arrays, null will be ignored. :)
array array_slice ( array $array , int $offset [, int $length = NULL [, bool $preserve_keys = FALSE ]] )
https://secure.php.net/manual/en/function.array-slice.php

nickvergessen · 2018-03-27T08:30:20Z

apps/provisioning_api/lib/Controller/UserDataTrait.php

+	 * @throws OCSException
+	 */
+	public function getUserData(string $userId): array {
+		$currentLoggedInUser = $this->userSession->getUser();


Hmm php storm doesn't know userSession. I guess you need to declare it in this trait too.

skjnldsv · 2018-04-03T12:20:07Z

Bump, failure unrelated (swift) :)

MorrisJobke · 2018-04-04T13:15:57Z

apps/provisioning_api/lib/Controller/GroupsController.php

+	 * @return DataResponse
+	 * @throws OCSException	
+	 *
+	 * @deprecated 14 Use getGroupUsers


Could you add a note about this to #7827? But I guess it's not easily possible to drop this as it is part of the OCS standard 😉

Yeah, I thought it was the better way. It's not that bad tough :)

Added to the changelog in #7827

MorrisJobke

Tested and works 👍

rullzer · 2018-04-04T20:25:43Z

CONFLICTS!!! ⚠️ 😱 🙀

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Signed-off-by: Joas Schilling <[email protected]>

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

skjnldsv · 2018-04-05T15:41:08Z

Failure unrelated: swift :)
@rullzer

rullzer

Lets do this!

skjnldsv added 3. to review Waiting for reviews feature: users and groups labels Mar 20, 2018

skjnldsv added this to the Nextcloud 14 milestone Mar 20, 2018

skjnldsv self-assigned this Mar 20, 2018

skjnldsv requested review from rullzer, nickvergessen, MorrisJobke, blizzz and juliusknorr March 20, 2018 18:08

skjnldsv mentioned this pull request Mar 21, 2018

Vue migration: settings #8824

Merged

34 tasks

nickvergessen previously requested changes Mar 22, 2018

View reviewed changes

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from cac9609 to 0db5c79 Compare March 22, 2018 14:40

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from 0db5c79 to e92f640 Compare March 22, 2018 14:52

skjnldsv requested a review from nickvergessen March 22, 2018 15:06

juliusknorr previously requested changes Mar 22, 2018

View reviewed changes

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch 8 times, most recently from f428a0b to d473653 Compare March 23, 2018 17:02

skjnldsv requested a review from juliusknorr March 23, 2018 17:25

skjnldsv commented Mar 23, 2018

View reviewed changes

nickvergessen reviewed Mar 26, 2018

View reviewed changes

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch 2 times, most recently from 4b4f4dc to 9b081c8 Compare March 27, 2018 07:48

nickvergessen reviewed Mar 27, 2018

View reviewed changes

nickvergessen force-pushed the ocs-api-get-users-details-per-groups branch from 1663ec5 to 5d13464 Compare March 27, 2018 09:07

MorrisJobke reviewed Apr 4, 2018

View reviewed changes

MorrisJobke approved these changes Apr 4, 2018

View reviewed changes

MorrisJobke mentioned this pull request Apr 4, 2018

Critical changes for developers & admins for Nextcloud 14 #7827

Closed

skjnldsv and others added 8 commits April 5, 2018 17:08

Api, return users details by groups

68ae439

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Limit and offset

f4e84e1

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Cast types directly

4011ea9

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fixed tests

b023bfe

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Trait bump

c103aaa

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fixed tests

762002e

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Use abstract subclass so members can be defined

348171f

Signed-off-by: Joas Schilling <[email protected]>

Rebase to master

eb4d7fb

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from 5d13464 to eb4d7fb Compare April 5, 2018 15:13

rullzer approved these changes Apr 6, 2018

View reviewed changes

rullzer merged commit 4a6e31c into master Apr 6, 2018

rullzer deleted the ocs-api-get-users-details-per-groups branch April 6, 2018 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api: return users details by groups #8904

Api: return users details by groups #8904

skjnldsv commented Mar 20, 2018 •

edited

Loading

nickvergessen Mar 22, 2018

nickvergessen Mar 22, 2018

codecov bot commented Mar 22, 2018 •

edited

Loading

skjnldsv commented Mar 22, 2018

juliusknorr Mar 22, 2018

skjnldsv Mar 23, 2018

skjnldsv Mar 23, 2018 •

edited

Loading

juliusknorr Mar 23, 2018

skjnldsv Mar 23, 2018

nickvergessen Mar 23, 2018

juliusknorr Mar 22, 2018

juliusknorr Mar 22, 2018

juliusknorr Mar 22, 2018

skjnldsv commented Mar 23, 2018 •

edited

Loading

skjnldsv Mar 23, 2018

MorrisJobke Apr 4, 2018

nickvergessen Mar 26, 2018

skjnldsv Mar 26, 2018

nickvergessen Mar 26, 2018

nickvergessen Mar 26, 2018

skjnldsv Mar 26, 2018

nickvergessen Mar 27, 2018

skjnldsv commented Apr 3, 2018

MorrisJobke Apr 4, 2018

skjnldsv Apr 4, 2018

MorrisJobke Apr 4, 2018

skjnldsv Apr 4, 2018

MorrisJobke left a comment

rullzer commented Apr 4, 2018

skjnldsv commented Apr 5, 2018 •

edited

Loading

rullzer left a comment

Api: return users details by groups #8904

Api: return users details by groups #8904

Conversation

skjnldsv commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2018 • edited Loading

Codecov Report

skjnldsv commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv commented Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

rullzer commented Apr 4, 2018

skjnldsv commented Apr 5, 2018 • edited Loading

rullzer left a comment

Choose a reason for hiding this comment

skjnldsv commented Mar 20, 2018 •

edited

Loading

codecov bot commented Mar 22, 2018 •

edited

Loading

skjnldsv Mar 23, 2018 •

edited

Loading

skjnldsv commented Mar 23, 2018 •

edited

Loading

skjnldsv commented Apr 5, 2018 •

edited

Loading